Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bevy_reflect: Add Reflectable trait #5772

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Aug 23, 2022

Objective

When deriving Reflect, users will notice that their generic arguments also need to implement Reflect:

#[derive(Reflect)]
struct Foo<T: Reflect> {
  value: T
}

This works well for now. However, as we want to do more with Reflect, these bounds might need to change. For example, to get #4154 working, we likely need to enforce the GetTypeRegistration trait. So now we have:

#[derive(Reflect)]
struct Foo<T: Reflect + GetTypeRegistration> {
  value: T
}

Not great, but not horrible. However, we might then want to do something as suggested in this comment and add a ReflectTypeName trait for stable type name support. Well now we have:

#[derive(Reflect)]
struct Foo<T: Reflect + GetTypeRegistration + ReflectTypeName> {
  value: T
}

Now imagine that for even two or three generic types. Yikes!

As the API changes it would be nice if users didn't need to manually migrate their generic type bounds like this.

A lot of these traits are (or will/might be) core to the entire reflection API. And although Reflect can't add them as supertraits for object-safety reasons, they are still indirectly required for things to function properly (manual implementors will know how easy it is to forget to implement GetTypeRegistration). And they should all be automatically implemented for user types anyways as long they're using #[derive(Reflect)].

Solution

Add a "catch-all" trait called Reflectable whose supertraits are a select handful of core reflection traits.

This allows us to consolidate all the examples above into this:

#[derive(Reflect)]
struct Foo<T: Reflectable> {
  value: T
}

And as we experiment with the API, users can rest easy knowing they don't need to migrate dozens upon dozens of types. It should all be automatic!

Discussion

  1. Thoughts on the name Reflectable? Is it too easily confused with Reflect? Or does it at least accurately describe that this contains the core traits? If not, maybe BaseReflect?

Changelog

  • Added the Reflectable trait

@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Aug 23, 2022
@alice-i-cecile
Copy link
Member

I'm in favor of this design: I've seen this sort of supertrait be super useful with Num.

Name is fine. Only blocking request is that this should be linked to from the Reflect docs (and any other traits that this subsumes).

@MrGVSV MrGVSV force-pushed the reflect-reflectable branch from 3e09a4e to 94814c3 Compare August 31, 2022 04:16
@@ -8,6 +8,8 @@ use std::any::{Any, TypeId};
/// This trait is automatically implemented by the `#[derive(Reflect)]` macro
/// and allows type information to be processed without an instance of that type.
///
/// As a core trait to the reflection system, it is a supertrait of [`Reflectable`].
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alice-i-cecile I'm not really sure I love this. Any thoughts or suggestions?

@MrGVSV MrGVSV force-pushed the reflect-reflectable branch from edbc673 to 0344bad Compare August 31, 2022 05:07
Copy link
Contributor

@afonsolage afonsolage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does advising users to make use of Reflectable will likely increas the number of breaking changes, whenever we update Reflectable? I mean, if I add a new trait to Reflectable, and it isn't an auto impl trait, this means the user will have to add a new #[derive] for all types that uses the generic bounds, right?

Also, should we consider using Reflectable as temporary until trait_alias stabilize (which may take years)?

@MrGVSV
Copy link
Member Author

MrGVSV commented Aug 31, 2022

Does advising users to make use of Reflectable will likely increas the number of breaking changes, whenever we update Reflectable? I mean, if I add a new trait to Reflectable, and it isn't an auto impl trait, this means the user will have to add a new #[derive] for all types that uses the generic bounds, right?

Reflect has the restriction that it needs to be object-safe. Because of this, we have a lot of other required (or partially-required) traits to provide other functionality. Typed is definitely required due to Reflect::get_type_info, while GetTypeRegistration is required if you need to use the registry. However, not implementing GetTypeRegistration can lead to subtle footguns.

Reflectable is meant to unify these core traits. And because these traits are so heavily utilized within bevy_reflect, they are automatically derived via #[derive(Reflect)].

I don't think Reflectable should contain anything that isn't auto-implemented via that derive macro. Therefore, adding a trait should only be a breaking change for those subset of users that implement Reflect (et al.) manually— which probably isn't many haha.

What could be a breaking change, though, is removing a trait. Anyone that used Reflectable to ensure a trait impl existed would suddenly not have that guarantee anymore. But if we removed a trait from Reflectable, we likely also removed it from the derive macro, meaning that this would still be a breaking change with-or-without Reflectable.

Also, should we consider using Reflectable as temporary until trait_alias stabilize (which may take years)?

I don't know much about it, but it looks like pretty much what we're trying to do. However, I'm not too sure what advantages it has over the blanket impl approach.

@alice-i-cecile
Copy link
Member

Reflectable is meant to unify these core traits. And because these traits are so heavily utilized within bevy_reflect, they are automatically derived via #[derive(Reflect)].

We should consider making the derive name the same as the name of the blanket trait. IMO that means:

  1. Reflect trait name is used for this Reflectable trait.
  2. The current Reflect functionality becomes something like BasicReflect.

@MrGVSV
Copy link
Member Author

MrGVSV commented Aug 31, 2022

We should consider making the derive name the same as the name of the blanket trait. IMO that means:

  • Reflect trait name is used for this Reflectable trait.
  • The current Reflect functionality becomes something like BasicReflect.

I don't mind this as it makes it slightly clearer that it will be deriving multiple traits (at least, if a user knows what Reflectable is). Or, at any rate I suppose it brings the trait to the attention of users.

However, I don't really like the idea of renaming Reflect to BaseReflect. All the other supertraits of Reflectable help drive the other parts of reflection. But most of that is utilized within bevy_reflect. Apart from generic types and functions where those bounds are necessary, users generally won't need to worry about them.

Most often, they'll just be working with dyn Reflect as opposed to a concrete generic type. As it stands right now, this PR shouldn't break anything on its own. If we changed to BaseReflect then the only thing users will see from it is that they need to use dyn BaseReflect now.

If we wanted to change anything, I feel it should at most be in your suggestion to make this the trait we "derive" via #[derive(Reflectable)]. But I think that could be saved for a more controversial followup PR haha.

@MrGVSV MrGVSV force-pushed the reflect-reflectable branch from 0344bad to be039c6 Compare September 21, 2022 19:27
@MrGVSV MrGVSV mentioned this pull request Jul 5, 2023
1 task
github-merge-queue bot pushed a commit that referenced this pull request Jan 28, 2024
# Objective

Fixes #8965.

#### Background

For convenience and to ensure everything is setup properly, we
automatically add certain bounds to the derived types. The current
implementation does this by taking the types from all active fields and
adding them to the where-clause of the generated impls. I believe this
method was chosen because it won't add bounds to types that are
otherwise ignored.

```rust
#[derive(Reflect)]
struct Foo<T, U: SomeTrait, V> {
  t: T,
  u: U::Assoc,
  #[reflect(ignore)]
  v: [V; 2]
}

// Generates something like:
impl<T, U: SomeTrait, V> for Foo<T, U, V>
where
  // Active:
  T: Reflect,
  U::Assoc: Reflect,

  // Ignored:
  [V; 2]: Send + Sync + Any
{
  // ...
}
```

The self-referential type fails because it ends up using _itself_ as a
type bound due to being one of its own active fields.

```rust
#[derive(Reflect)]
struct Foo {
  foo: Vec<Foo>
}

// Foo where Vec<Foo>: Reflect -> Vec<T> where T: Reflect -> Foo where Vec<Foo>: Reflect -> ...
```

## Solution

We can't simply parse all field types for the name of our type. That
would be both complex and prone to errors and false-positives. And even
if it wasn't, what would we replace the bound with?

Instead, I opted to go for a solution that only adds the bounds to what
really needs it: the type parameters. While the bounds on concrete types
make errors a bit cleaner, they aren't strictly necessary. This means we
can change our generated where-clause to only add bounds to generic type
parameters.

Doing this, though, returns us back to the problem of over-bounding
parameters that don't need to be bounded. To solve this, I added a new
container attribute (based on
[this](dtolnay/syn#422 (comment))
comment and @nicopap's
[comment](#9046 (comment)))
that allows us to pass in a custom where clause to modify what bounds
are added to these type parameters.

This allows us to do stuff like:

```rust
trait Trait {
  type Assoc;
}

// We don't need `T` to be reflectable since we only care about `T::Assoc`.
#[derive(Reflect)]
#[reflect(where T::Assoc: FromReflect)]
struct Foo<T: Trait>(T::Assoc);

#[derive(TypePath)]
struct Bar;

impl Trait for Bar {
  type Assoc = usize;
}

#[derive(Reflect)]
struct Baz {
  a: Foo<Bar>,
}
```

> **Note**
> I also
[tried](dc139ea)
allowing `#[reflect(ignore)]` to be used on the type parameters
themselves, but that proved problematic since the derive macro does not
consume the attribute. This is why I went with the container attribute
approach.

### Alternatives

One alternative could possibly be to just not add reflection bounds
automatically (i.e. only add required bounds like `Send`, `Sync`, `Any`,
and `TypePath`).

The downside here is we add more friction to using reflection, which
already comes with its own set of considerations. This is a potentially
viable option, but we really need to consider whether or not the
ergonomics hit is worth it.

If we did decide to go the more manual route, we should at least
consider something like #5772 to make it easier for users to add the
right bounds (although, this could still become tricky with
`FromReflect` also being automatically derived).

### Open Questions

1. Should we go with this approach or the manual alternative?
2. ~~Should we add a `skip_params` attribute to avoid the `T: 'static`
trick?~~ ~~Decided to go with `custom_where()` as it's the simplest~~
Scratch that, went with a normal where clause
3. ~~`custom_where` bikeshedding?~~ No longer needed since we are using
a normal where clause

### TODO

- [x] Add compile-fail tests

---

## Changelog

- Fixed issue preventing recursive types from deriving `Reflect`
- Changed how where-clause bounds are generated by the `Reflect` derive
macro
- They are now only applied to the type parameters, not to all active
fields
- Added `#[reflect(where T: Trait, U::Assoc: Trait, ...)]` container
attribute

## Migration Guide

When deriving `Reflect`, generic type params that do not need the
automatic reflection bounds (such as `Reflect`) applied to them will
need to opt-out using a custom where clause like: `#[reflect(where T:
Trait, U::Assoc: Trait, ...)]`.

The attribute can define custom bounds only used by the reflection
impls. To simply opt-out all the type params, we can pass in an empty
where clause: `#[reflect(where)]`.

```rust
// BEFORE:
#[derive(Reflect)]
struct Foo<T>(#[reflect(ignore)] T);

// AFTER:
#[derive(Reflect)]
#[reflect(where)]
struct Foo<T>(#[reflect(ignore)] T);
```

---------

Co-authored-by: Nicola Papale <[email protected]>
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Fixes bevyengine#8965.

#### Background

For convenience and to ensure everything is setup properly, we
automatically add certain bounds to the derived types. The current
implementation does this by taking the types from all active fields and
adding them to the where-clause of the generated impls. I believe this
method was chosen because it won't add bounds to types that are
otherwise ignored.

```rust
#[derive(Reflect)]
struct Foo<T, U: SomeTrait, V> {
  t: T,
  u: U::Assoc,
  #[reflect(ignore)]
  v: [V; 2]
}

// Generates something like:
impl<T, U: SomeTrait, V> for Foo<T, U, V>
where
  // Active:
  T: Reflect,
  U::Assoc: Reflect,

  // Ignored:
  [V; 2]: Send + Sync + Any
{
  // ...
}
```

The self-referential type fails because it ends up using _itself_ as a
type bound due to being one of its own active fields.

```rust
#[derive(Reflect)]
struct Foo {
  foo: Vec<Foo>
}

// Foo where Vec<Foo>: Reflect -> Vec<T> where T: Reflect -> Foo where Vec<Foo>: Reflect -> ...
```

## Solution

We can't simply parse all field types for the name of our type. That
would be both complex and prone to errors and false-positives. And even
if it wasn't, what would we replace the bound with?

Instead, I opted to go for a solution that only adds the bounds to what
really needs it: the type parameters. While the bounds on concrete types
make errors a bit cleaner, they aren't strictly necessary. This means we
can change our generated where-clause to only add bounds to generic type
parameters.

Doing this, though, returns us back to the problem of over-bounding
parameters that don't need to be bounded. To solve this, I added a new
container attribute (based on
[this](dtolnay/syn#422 (comment))
comment and @nicopap's
[comment](bevyengine#9046 (comment)))
that allows us to pass in a custom where clause to modify what bounds
are added to these type parameters.

This allows us to do stuff like:

```rust
trait Trait {
  type Assoc;
}

// We don't need `T` to be reflectable since we only care about `T::Assoc`.
#[derive(Reflect)]
#[reflect(where T::Assoc: FromReflect)]
struct Foo<T: Trait>(T::Assoc);

#[derive(TypePath)]
struct Bar;

impl Trait for Bar {
  type Assoc = usize;
}

#[derive(Reflect)]
struct Baz {
  a: Foo<Bar>,
}
```

> **Note**
> I also
[tried](bevyengine@dc139ea)
allowing `#[reflect(ignore)]` to be used on the type parameters
themselves, but that proved problematic since the derive macro does not
consume the attribute. This is why I went with the container attribute
approach.

### Alternatives

One alternative could possibly be to just not add reflection bounds
automatically (i.e. only add required bounds like `Send`, `Sync`, `Any`,
and `TypePath`).

The downside here is we add more friction to using reflection, which
already comes with its own set of considerations. This is a potentially
viable option, but we really need to consider whether or not the
ergonomics hit is worth it.

If we did decide to go the more manual route, we should at least
consider something like bevyengine#5772 to make it easier for users to add the
right bounds (although, this could still become tricky with
`FromReflect` also being automatically derived).

### Open Questions

1. Should we go with this approach or the manual alternative?
2. ~~Should we add a `skip_params` attribute to avoid the `T: 'static`
trick?~~ ~~Decided to go with `custom_where()` as it's the simplest~~
Scratch that, went with a normal where clause
3. ~~`custom_where` bikeshedding?~~ No longer needed since we are using
a normal where clause

### TODO

- [x] Add compile-fail tests

---

## Changelog

- Fixed issue preventing recursive types from deriving `Reflect`
- Changed how where-clause bounds are generated by the `Reflect` derive
macro
- They are now only applied to the type parameters, not to all active
fields
- Added `#[reflect(where T: Trait, U::Assoc: Trait, ...)]` container
attribute

## Migration Guide

When deriving `Reflect`, generic type params that do not need the
automatic reflection bounds (such as `Reflect`) applied to them will
need to opt-out using a custom where clause like: `#[reflect(where T:
Trait, U::Assoc: Trait, ...)]`.

The attribute can define custom bounds only used by the reflection
impls. To simply opt-out all the type params, we can pass in an empty
where clause: `#[reflect(where)]`.

```rust
// BEFORE:
#[derive(Reflect)]
struct Foo<T>(#[reflect(ignore)] T);

// AFTER:
#[derive(Reflect)]
#[reflect(where)]
struct Foo<T>(#[reflect(ignore)] T);
```

---------

Co-authored-by: Nicola Papale <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Mar 4, 2024
# Objective

Resolves #4154

Currently, registration must all be done manually:

```rust
#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app
    .register_type::<Foo>()
    .register_type::<Bar>()
    .register_type::<Baz>()
    // .register_type::<usize>() <- This one is handled by Bevy, thankfully
  // ...
}
```

This can grow really quickly and become very annoying to add, remove,
and update as types change. It would be great if we could help reduce
the number of types that a user must manually implement themselves.

## Solution

As suggested in #4154, this PR adds automatic recursive registration.
Essentially, when a type is registered, it may now also choose to
register additional types along with it using the new
`GetTypeRegistration::register_type_dependencies` trait method.

The `Reflect` derive macro now automatically does this for all fields in
structs, tuple structs, struct variants, and tuple variants. This is
also done for tuples, arrays, `Vec<T>`, `HashMap<K, V>`, and
`Option<T>`.

This allows us to simplify the code above like:

```rust
#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app.register_type::<Foo>()
  // ...
}
```

This automatic registration only occurs if the type has not yet been
registered. If it has been registered, we simply skip it and move to the
next one. This reduces the cost of registration and prevents overwriting
customized registrations.

## Considerations

While this does improve ergonomics on one front, it's important to look
at some of the arguments against adopting a PR like this.

#### Generic Bounds

~~Since we need to be able to register the fields individually, we need
those fields to implement `GetTypeRegistration`. This forces users to
then add this trait as a bound on their generic arguments. This
annoyance could be relieved with something like #5772.~~

This is no longer a major issue as the `Reflect` derive now adds the
`GetTypeRegistration` bound by default. This should technically be okay,
since we already add the `Reflect` bound.

However, this can also be considered a breaking change for manual
implementations that left out a `GetTypeRegistration` impl ~~or for
items that contain dynamic types (e.g. `DynamicStruct`) since those also
do not implement `GetTypeRegistration`~~.

#### Registration Assumptions

By automatically registering fields, users might inadvertently be
relying on certain types to be automatically registered. If `Foo`
auto-registers `Bar`, but `Foo` is later removed from the code, then
anywhere that previously used or relied on `Bar`'s registration would
now fail.

---

## Changelog

- Added recursive type registration to structs, tuple structs, struct
variants, tuple variants, tuples, arrays, `Vec<T>`, `HashMap<K, V>`, and
`Option<T>`
- Added a new trait in the hidden `bevy_reflect::__macro_exports` module
called `RegisterForReflection`
- Added `GetTypeRegistration` impl for
`bevy_render::render_asset::RenderAssetUsages`

## Migration Guide

All types that derive `Reflect` will now automatically add
`GetTypeRegistration` as a bound on all (unignored) fields. This means
that all reflected fields will need to also implement
`GetTypeRegistration`.

If all fields **derive** `Reflect` or are implemented in `bevy_reflect`,
this should not cause any issues. However, manual implementations of
`Reflect` that excluded a `GetTypeRegistration` impl for their type will
need to add one.

```rust
#[derive(Reflect)]
struct Foo<T: FromReflect> {
  data: MyCustomType<T>
}

// OLD
impl<T: FromReflect> Reflect for MyCustomType<T> {/* ... */}

// NEW
impl<T: FromReflect + GetTypeRegistration> Reflect for MyCustomType<T> {/* ... */}
impl<T: FromReflect + GetTypeRegistration> GetTypeRegistration for MyCustomType<T> {/* ... */}
```

---------

Co-authored-by: James Liu <[email protected]>
Co-authored-by: radiish <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
# Objective

Resolves bevyengine#4154

Currently, registration must all be done manually:

```rust
#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app
    .register_type::<Foo>()
    .register_type::<Bar>()
    .register_type::<Baz>()
    // .register_type::<usize>() <- This one is handled by Bevy, thankfully
  // ...
}
```

This can grow really quickly and become very annoying to add, remove,
and update as types change. It would be great if we could help reduce
the number of types that a user must manually implement themselves.

## Solution

As suggested in bevyengine#4154, this PR adds automatic recursive registration.
Essentially, when a type is registered, it may now also choose to
register additional types along with it using the new
`GetTypeRegistration::register_type_dependencies` trait method.

The `Reflect` derive macro now automatically does this for all fields in
structs, tuple structs, struct variants, and tuple variants. This is
also done for tuples, arrays, `Vec<T>`, `HashMap<K, V>`, and
`Option<T>`.

This allows us to simplify the code above like:

```rust
#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app.register_type::<Foo>()
  // ...
}
```

This automatic registration only occurs if the type has not yet been
registered. If it has been registered, we simply skip it and move to the
next one. This reduces the cost of registration and prevents overwriting
customized registrations.

## Considerations

While this does improve ergonomics on one front, it's important to look
at some of the arguments against adopting a PR like this.

#### Generic Bounds

~~Since we need to be able to register the fields individually, we need
those fields to implement `GetTypeRegistration`. This forces users to
then add this trait as a bound on their generic arguments. This
annoyance could be relieved with something like bevyengine#5772.~~

This is no longer a major issue as the `Reflect` derive now adds the
`GetTypeRegistration` bound by default. This should technically be okay,
since we already add the `Reflect` bound.

However, this can also be considered a breaking change for manual
implementations that left out a `GetTypeRegistration` impl ~~or for
items that contain dynamic types (e.g. `DynamicStruct`) since those also
do not implement `GetTypeRegistration`~~.

#### Registration Assumptions

By automatically registering fields, users might inadvertently be
relying on certain types to be automatically registered. If `Foo`
auto-registers `Bar`, but `Foo` is later removed from the code, then
anywhere that previously used or relied on `Bar`'s registration would
now fail.

---

## Changelog

- Added recursive type registration to structs, tuple structs, struct
variants, tuple variants, tuples, arrays, `Vec<T>`, `HashMap<K, V>`, and
`Option<T>`
- Added a new trait in the hidden `bevy_reflect::__macro_exports` module
called `RegisterForReflection`
- Added `GetTypeRegistration` impl for
`bevy_render::render_asset::RenderAssetUsages`

## Migration Guide

All types that derive `Reflect` will now automatically add
`GetTypeRegistration` as a bound on all (unignored) fields. This means
that all reflected fields will need to also implement
`GetTypeRegistration`.

If all fields **derive** `Reflect` or are implemented in `bevy_reflect`,
this should not cause any issues. However, manual implementations of
`Reflect` that excluded a `GetTypeRegistration` impl for their type will
need to add one.

```rust
#[derive(Reflect)]
struct Foo<T: FromReflect> {
  data: MyCustomType<T>
}

// OLD
impl<T: FromReflect> Reflect for MyCustomType<T> {/* ... */}

// NEW
impl<T: FromReflect + GetTypeRegistration> Reflect for MyCustomType<T> {/* ... */}
impl<T: FromReflect + GetTypeRegistration> GetTypeRegistration for MyCustomType<T> {/* ... */}
```

---------

Co-authored-by: James Liu <[email protected]>
Co-authored-by: radiish <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
mtsr pushed a commit to mtsr/bevy that referenced this pull request Mar 15, 2024
# Objective

Resolves bevyengine#4154

Currently, registration must all be done manually:

```rust
#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app
    .register_type::<Foo>()
    .register_type::<Bar>()
    .register_type::<Baz>()
    // .register_type::<usize>() <- This one is handled by Bevy, thankfully
  // ...
}
```

This can grow really quickly and become very annoying to add, remove,
and update as types change. It would be great if we could help reduce
the number of types that a user must manually implement themselves.

## Solution

As suggested in bevyengine#4154, this PR adds automatic recursive registration.
Essentially, when a type is registered, it may now also choose to
register additional types along with it using the new
`GetTypeRegistration::register_type_dependencies` trait method.

The `Reflect` derive macro now automatically does this for all fields in
structs, tuple structs, struct variants, and tuple variants. This is
also done for tuples, arrays, `Vec<T>`, `HashMap<K, V>`, and
`Option<T>`.

This allows us to simplify the code above like:

```rust
#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app.register_type::<Foo>()
  // ...
}
```

This automatic registration only occurs if the type has not yet been
registered. If it has been registered, we simply skip it and move to the
next one. This reduces the cost of registration and prevents overwriting
customized registrations.

## Considerations

While this does improve ergonomics on one front, it's important to look
at some of the arguments against adopting a PR like this.

#### Generic Bounds

~~Since we need to be able to register the fields individually, we need
those fields to implement `GetTypeRegistration`. This forces users to
then add this trait as a bound on their generic arguments. This
annoyance could be relieved with something like bevyengine#5772.~~

This is no longer a major issue as the `Reflect` derive now adds the
`GetTypeRegistration` bound by default. This should technically be okay,
since we already add the `Reflect` bound.

However, this can also be considered a breaking change for manual
implementations that left out a `GetTypeRegistration` impl ~~or for
items that contain dynamic types (e.g. `DynamicStruct`) since those also
do not implement `GetTypeRegistration`~~.

#### Registration Assumptions

By automatically registering fields, users might inadvertently be
relying on certain types to be automatically registered. If `Foo`
auto-registers `Bar`, but `Foo` is later removed from the code, then
anywhere that previously used or relied on `Bar`'s registration would
now fail.

---

## Changelog

- Added recursive type registration to structs, tuple structs, struct
variants, tuple variants, tuples, arrays, `Vec<T>`, `HashMap<K, V>`, and
`Option<T>`
- Added a new trait in the hidden `bevy_reflect::__macro_exports` module
called `RegisterForReflection`
- Added `GetTypeRegistration` impl for
`bevy_render::render_asset::RenderAssetUsages`

## Migration Guide

All types that derive `Reflect` will now automatically add
`GetTypeRegistration` as a bound on all (unignored) fields. This means
that all reflected fields will need to also implement
`GetTypeRegistration`.

If all fields **derive** `Reflect` or are implemented in `bevy_reflect`,
this should not cause any issues. However, manual implementations of
`Reflect` that excluded a `GetTypeRegistration` impl for their type will
need to add one.

```rust
#[derive(Reflect)]
struct Foo<T: FromReflect> {
  data: MyCustomType<T>
}

// OLD
impl<T: FromReflect> Reflect for MyCustomType<T> {/* ... */}

// NEW
impl<T: FromReflect + GetTypeRegistration> Reflect for MyCustomType<T> {/* ... */}
impl<T: FromReflect + GetTypeRegistration> GetTypeRegistration for MyCustomType<T> {/* ... */}
```

---------

Co-authored-by: James Liu <[email protected]>
Co-authored-by: radiish <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Aug 12, 2024
# Objective

- Implements the [Unique Reflect
RFC](https://github.com/nicopap/rfcs/blob/bevy-reflect-api/rfcs/56-better-reflect.md).

## Solution

- Implements the RFC.
- This implementation differs in some ways from the RFC:
- In the RFC, it was suggested `Reflect: Any` but `PartialReflect:
?Any`. During initial implementation I tried this, but we assume the
`PartialReflect: 'static` in a lot of places and the changes required
crept out of the scope of this PR.
- `PartialReflect::try_into_reflect` originally returned `Option<Box<dyn
Reflect>>` but i changed this to `Result<Box<dyn Reflect>, Box<dyn
PartialReflect>>` since the method takes by value and otherwise there
would be no way to recover the type. `as_full` and `as_full_mut` both
still return `Option<&(mut) dyn Reflect>`.

---

## Changelog

- Added `PartialReflect`.
- `Reflect` is now a subtrait of `PartialReflect`.
- Moved most methods on `Reflect` to the new `PartialReflect`.
- Added `PartialReflect::{as_partial_reflect, as_partial_reflect_mut,
into_partial_reflect}`.
- Added `PartialReflect::{try_as_reflect, try_as_reflect_mut,
try_into_reflect}`.
- Added `<dyn PartialReflect>::{try_downcast_ref, try_downcast_mut,
try_downcast, try_take}` supplementing the methods on `dyn Reflect`.

## Migration Guide

- Most instances of `dyn Reflect` should be changed to `dyn
PartialReflect` which is less restrictive, however trait bounds should
generally stay as `T: Reflect`.
- The new `PartialReflect::{as_partial_reflect, as_partial_reflect_mut,
into_partial_reflect, try_as_reflect, try_as_reflect_mut,
try_into_reflect}` methods as well as `Reflect::{as_reflect,
as_reflect_mut, into_reflect}` will need to be implemented for manual
implementors of `Reflect`.

## Future Work

- This PR is designed to be followed up by another "Unique Reflect Phase
2" that addresses the following points:
- Investigate making serialization revolve around `Reflect` instead of
`PartialReflect`.
- [Remove the `try_*` methods on `dyn PartialReflect` since they are
stop
gaps](#7207 (comment)).
- Investigate usages like `ReflectComponent`. In the places they
currently use `PartialReflect`, should they be changed to use `Reflect`?
- Merging this opens the door to lots of reflection features we haven't
been able to implement.
- We could re-add [the `Reflectable`
trait](https://github.com/bevyengine/bevy/blob/8e3488c88065a94a4f72199587e59341c9b6553d/crates/bevy_reflect/src/reflect.rs#L337-L342)
and make `FromReflect` a requirement to improve [`FromReflect`
ergonomics](bevyengine/rfcs#59). This is
currently not possible because dynamic types cannot sensibly be
`FromReflect`.
  - Since this is an alternative to #5772, #5781 would be made cleaner.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Gino Valente <[email protected]>
@MrGVSV MrGVSV force-pushed the reflect-reflectable branch from be039c6 to 355f71a Compare September 17, 2024 22:55
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward M-Needs-Release-Note Work that should be called out in the blog due to impact labels Sep 17, 2024
Copy link
Contributor

@brandon-reinhart brandon-reinhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks straightforward to me.

Is FromReflect also needed?

At the moment, I need:

pub trait EntityLocator:
    GetTypeRegistration + Typed + FromReflect + Reflect + TypePath

To do this:

    fn register_entity_locator<Locator: EntityLocator>(&mut self) -> &mut Self {
        self.register_type::<MapLocations<Locator>>();
        
       // other stuff
      }

Without which I get:

error[E0277]: `Locator` can not be created through reflection
   --> game\tier1\src\ai\locations\map_locations.rs:11:30
    |
11  |         self.register_type::<MapLocations<Locator>>();
    |              -------------   ^^^^^^^^^^^^^^^^^^^^^ the trait `bevy::prelude::FromReflect` is not implemented for `Locator`, which is required by `MapLocations<Locator>: bevy::bevy_reflect::GetTypeRegistration`
    |              |
    |              required by a bound introduced by this call
    |
    = note: consider annotating `Locator` with `#[derive(FromReflect)]`

@MrGVSV
Copy link
Member Author

MrGVSV commented Sep 17, 2024

This looks straightforward to me.

Is FromReflect also needed?

Good question! I chose to leave FromReflect out since it's not strictly required by reflection. It can be opted out of by the derive macro, which would make Reflectable a lot more restrictive than would probably be ideal.

So in the cases you need FromReflect, I'd suggest just making that part explicit:

pub trait EntityLocator: Reflectable + FromReflect {}

@MrGVSV MrGVSV force-pushed the reflect-reflectable branch from 355f71a to d64ecb8 Compare September 17, 2024 23:59
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 18, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a wording suggestion, but I think this is useful and clear.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 18, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 18, 2024
Merged via the queue into bevyengine:main with commit 6954146 Sep 18, 2024
26 checks passed
@MrGVSV MrGVSV deleted the reflect-reflectable branch September 18, 2024 00:54
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1687 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants